r? @thomcc

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Wouldn't using one of HashMap::with_capacity(), HashMap::reserve(), FromIterator<(K, V)>, Extend<(K, V)> or From<[(K, V); N]> be better, avoiding unnecessary allocations?

Wouldn't using one of HashMap::with_capacity(), HashMap::reserve(), FromIterator<(K, V)>, Extend<(K, V)> or From<[(K, V); N]> be better, avoiding unnecessary allocations?

I was thinking about HashMap::with_capacity() but I couldn't see how I could convert a repetition into a number.

From... would essentially be the same, just reallocation, so for clarity I just expanded into insert.

I could see that 0 $(+ 1)* could be evaluated constantly by the compiler, thus we could get a size, but I'm not sure if it's the best way.
It could not be the best way, because that expansion doesn't at all include a reference to the repetition declaration, so it can't be. There is no logical way of getting the size with a macro_rules unless with more complexity we make a recursive macro, which would limit the hash_map size to 254 without extra compiler parameters.

I will add a question about the size in the tracking issue.

All the options i mentioned should be equivalent to with_capacity as far as I can tell, though I don't know if the optimizer can look through all options equally well (e.g. wrt. stack usage).
Both these should work, for example:

let len = 1 $(+ {stringify!($k); 1})*;
let mut map = HashMap::with_capacity(len);
$( map.insert($k, $v); )*

HashMap::from([$(($k, $v)),*])

All the options i mentioned should be equivalent to with_capacity as far as I can tell, though I don't know if the optimizer can look through all options equally well (e.g. wrt. stack usage). Both these should work, for example:

let len = 1 $(+ {stringify!($k); 1})*;
let mut map = HashMap::with_capacity(len);
$( map.insert($k, $v); )*

HashMap::from([$(($k, $v)),*])

I think I'd rather go for intrinsics, except since HashMap is a wrapper, I don't think there are many. I feel that if someone decides to expand the macro via IDE or cargo expand, they will find a lot of strings and be like "???". Probably open an issue later.

Otherwise, we could generate a constant count probably, the compiler should optimize this, and it should not have runtime cost, but a bit extra of constant cost, which may mean that if there are a lot of invocations the compiler might go a bit slower.

macro_rules! hash_map {
    () => {{
        ::std::collections::HashMap::new()
    }};

    ( $( $key:expr => $value:expr ),* $(,)? ) => {{
        let mut map = ::std::collections::HashMap::with_capacity(
            const { [$({ let _ = &$key; () }),*].len() }
        );

        $( map.insert($key, $value); )*
        map
    }}
}

I don't think intrinsics make sense here, this is a library type. I think the repeat insert method makes the most sense as it doesn't have the potential stack usage problem of from. Repeat insert is also how extend is implemented under the hood: https://github.com/rust-lang/hashbrown/blob/670213fb32d208759d0331996894e05604de0c18/src/map.rs#L4562.

with_capacity definitely makes sense though. You can count the elements like this: https://lukaswirth.dev/tlborm/decl-macros/building-blocks/counting.html

Oh actually you might be able to use the unstable macro_metavar_expr_count for the counting too, I forgot about that :D

I don't think intrinsics make sense here, this is a library type. I think the repeat insert method makes the most sense as it doesn't have the potential stack usage problem of from. Repeat insert is also how extend is implemented under the hood: https://github.com/rust-lang/hashbrown/blob/670213fb32d208759d0331996894e05604de0c18/src/map.rs#L4562.

with_capacity definitely makes sense though. You can count the elements like this: https://lukaswirth.dev/tlborm/decl-macros/building-blocks/counting.html

I would go for the slice length approach, but resolved constantly, which is technically what I'm doing just that instead of making a variable using replace_expr!

So probably

macro_rules! hash_map {
    () => {{
        ::std::collections::HashMap::new()
    }};

    ( $( $key:expr => $value:expr ),* $(,)? ) => {{
        let mut map = ::std::collections::HashMap::with_capacity(
            const { [$($crate::hash_map!(@internal count $key)),*].len() }
        );

        $( map.insert($key, $value); )*
        map
    }};

    (@internal count $i:tt) => {()}
}

This would work best if there is some sort of thing inside the standard library so that the last branch is only used internally in an explicit way, which would also filter out the usage inside the second branch.

Oh actually you might be able to use the unstable macro_metavar_expr_count for the counting too, I forgot about that :D

You should try this, which I only remembered after posting the first message

Oh actually you might be able to use the unstable macro_metavar_expr_count for the counting too, I forgot about that :D

You should try this, which I only remembered after posting the first message

The problem with this is that it would delay stabilization a lot, so I'm not sure, I don't think adding unstable to unstable helps

You're allowed to use unstable features internally with #[allow_internal_unstable()] and listing out the feature names there, on the macro.

Would it be possible to use HashMap::default() instead of HashMap::new() so that this macro can be used with non-default hashing algorithms as well? (Though this might cause type inference problems...)

EDIT: or rather

HashMap::with_capacity_and_hasher(capacity, Default::default())

for specifying capacity.

You're allowed to use unstable features internally with #[allow_internal_unstable()] and listing out the feature names there, on the macro.

Ok, I will check that now